Skip to content

Conversation

@jvz
Copy link
Member

@jvz jvz commented Oct 10, 2025

As discovered in issue #3947, an NPE can be thrown when an SslConfiguration returns null for its keystore, truststore, or SSLContext, in particular, in SslSocketManager. Other fixes are noted by IntelliJ's nullability analysis on some files I checked that use SslConfiguration instances.

This fixes #3947.

As discovered in issue #3947, an NPE can be thrown when an `SslConfiguration` returns `null` for its keystore, truststore, or `SSLContext`, in particular, in `SslSocketManager`.
@vy
Copy link
Member

vy commented Oct 11, 2025

@jvz, thanks so much for the prompt and clean work. Would you mind adding a test reproducing the issue reported in #3947, please?

@jvz
Copy link
Member Author

jvz commented Oct 13, 2025

@jvz, thanks so much for the prompt and clean work. Would you mind adding a test reproducing the issue reported in #3947, please?

Would you prefer this in a unit test fashion or as a functional test? I can see the value in either. The unit tests would be useful around ensuring things work with null values, but a functional test could check that you can use a trust store without a key store for syslog appenders.

@vy
Copy link
Member

vy commented Oct 13, 2025

Would you prefer this in a unit test fashion or as a functional test? I can see the value in either. The unit tests would be useful around ensuring things work with null values, but a functional test could check that you can use a trust store without a key store for syslog appenders.

I'd appreciate a unit test reproducing the failure reported in #3947 – it doesn't necessarily need to be exactly the same, but should dial effectively same eventual knobs to trigger it. To aid with the implementation, feel free to mock stuff that is unrelated for the test subject. You can read this as, "Please no Docker ITs, etc. Just simple JUnit tests I can run using ./mvnw test -Dtest=FooTest." While reviewing, what I have in mind is to remove your fix, and confirm that the test fails in the same fashion as reported. I'm assuming test passes with your fix. 😅

@jvz jvz requested a review from vy October 16, 2025 17:15
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jvz, the test (i.e., SslSocketManagerTest) you added in 5462cfb passes on 2.x without your changes. Would you mind providing a test that fails without your changes, and passes with your changes, please?

@jvz
Copy link
Member Author

jvz commented Oct 24, 2025

@vy I found the right spot to test

@jvz jvz requested a review from vy October 24, 2025 17:42
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that test fails on 2.x with the error reported in #3947.

@vy vy enabled auto-merge (squash) October 24, 2025 18:57
@vy vy merged commit 9643c22 into 2.x Oct 24, 2025
7 checks passed
@vy vy deleted the fix/2.x/syslog-truststore branch October 24, 2025 19:15
@github-project-automation github-project-automation bot moved this from To triage to Done in Log4j bug tracker Oct 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Sending Syslog via TLS not possible with only encryted TLS Channel without CERT AUTH

2 participants